Skip to content

daemon: parse incrementally in dlt_daemon_control_set_trace_status_v2 (V2 control-handler family, ref #866)#869

Open
SoundMatt wants to merge 1 commit into
COVESA:masterfrom
SoundMatt:fix/set-trace-status-v2-parse-incremental
Open

daemon: parse incrementally in dlt_daemon_control_set_trace_status_v2 (V2 control-handler family, ref #866)#869
SoundMatt wants to merge 1 commit into
COVESA:masterfrom
SoundMatt:fix/set-trace-status-v2-parse-incremental

Conversation

@SoundMatt
Copy link
Copy Markdown

Problem

dlt_daemon_control_set_trace_status_v2 was the most severe of the
V2 control-handler bugs documented in #866 — it cast msg->databuffer
directly to DltServiceSetLogLevelV2 *:

req = (DltServiceSetLogLevelV2 *)(msg->databuffer);
...
dlt_set_id_v2(apid, req->apid, req->apidlen);

DltServiceSetLogLevelV2 contains char *apid and char *ctid
pointer fields. Under the direct cast, those fields were filled with
8 bytes of network input each (interpreted as pointer values), then
dereferenced by dlt_set_id_v2. Arbitrary memory read from network
input.
The daemon either crashed (most pointers invalid) or read
from an attacker-chosen address.

Plus two compounding bugs:

Reachable from any client able to send control messages (dispatched
from dlt_daemon_handle_control_messages_v2, line 1278).

Fix

Replace the struct cast with incremental parsing, mirroring the
surgical approach already taken for set_log_level_v2 in #864:

  • Read each fixed-size field via memcpy into a local primitive.
  • Point apid / ctid into msg->databuffer only when their
    corresponding length is non-zero.
  • Add bounds checks after each length is parsed, mirroring daemon: fix out-of-bounds read in dlt_daemon_control_set_log_level_v2 #862.
  • Use unsigned widths throughout this function — drop the
    (int8_t) casts on apid_length / ctid_length.

Notes

  • Downstream helpers (dlt_daemon_find_multiple_context_and_send_trace_status_v2,
    dlt_daemon_send_trace_status_v2) still take int8_t length
    parameters. That narrowing remains a separate latent issue — a
    caller passing apidlen > 127 (legal in the wire protocol's
    uint8_t range) still gets a sign-flipped length passed
    downstream. Called out in a code comment; out of scope for this
    PR. Will follow up if maintainers want.
  • Same hand-written control flow as the original (wildcard / "field
    only" / "exact match" branches) — only the parsing and the local
    variable widths changed.
  • Tested by reading the diff and the V2 protocol layout against the
    size constant DLT_SERVICE_SET_LOG_LEVEL_FIXED_SIZE_V2 = 11. Have
    not added a regression test.

Related: #866 (META — V2 control-handler systemic problem).
Mirrors: #864 (same surgical approach for set_log_level_v2).

This handler had three compounding bugs from the V2 control-handler
pattern documented in COVESA#866:

1. Direct cast of msg->databuffer to DltServiceSetLogLevelV2 *.
   That struct contains `char *apid` and `char *ctid` pointer fields,
   so under the cast those fields took attacker-controlled pointer
   values from network input. dlt_set_id_v2(apid, req->apid, ...)
   then read from req->apid — an arbitrary memory read.

2. Same NULL-deref pattern as COVESA#863 / COVESA#864: locals apid / ctid declared
   NULL, dlt_set_id_v2(apid, ...) silently no-ops on the NULL
   destination, then apid[apid_length - 1] dereferences NULL when
   apid_length != 0.

3. (int8_t) cast on uint8_t req->apidlen / ctidlen produces a negative
   value when the length exceeds 127, then apid[apid_length - 1]
   underflows to a negative array index.

Fix: parse the wire bytes incrementally into local primitives, point
apid / ctid into msg->databuffer when their lengths are non-zero,
add bounds checks (mirroring COVESA#862 for set_log_level_v2), and use
unsigned widths in this function. Downstream helpers still take
int8_t lengths — that narrowing is a separate latent issue, called
out in a code comment, out of scope here.

Related: COVESA#866. Mirrors the surgical approach taken in COVESA#864.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant